Skip to content

Conversation

elhimov
Copy link

@elhimov elhimov commented Oct 17, 2025

I didn't forget about (remove if it is not applicable):

Related issues:

Closes TNTP-3733

@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch from e796751 to 4dd55cd Compare October 17, 2025 09:53
@coveralls
Copy link

coveralls commented Oct 17, 2025

Coverage Status

coverage: 91.616%. remained the same
when pulling e6db0a0 on elhimov/tntp-3733-import-and-actualize-benchmarks
into d70dadd on master.

@elhimov elhimov requested review from AlexandrLitkevich, bigbes and oleg-jukovec and removed request for oleg-jukovec October 17, 2025 09:59
@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch 6 times, most recently from ece055f to fdd30f4 Compare October 20, 2025 12:05
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, fix the red CI.

.PHONY: bench
bench:
@echo "Running benchmarks"
@go test ./... -count=1 -bench=. -benchmem
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need the benchmem? If so - ok.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-benchmem will show allocation count, I think it's necessary information for us to use.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are interested in number of allocation and this option displays allocation statistic.

dec := msgpack.GetDecoder()
dec.Reset(&buf)

b.Run("Optional1Bench", func(b *testing.B) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it a go-idiomic practice to name the tests in the manner? Why do we need the CamelCase here? Why not just "optional simple bench" or so on?

Up to @bigbes .

Copy link
Collaborator

@bigbes bigbes Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I don't have any strong opinion about naming of subtests. On one hand I'm preferring to have name that I can copy to cli to run subtest, but go-idiomatic way is to write clear names with spaces in lowercase.

Let's stick with go-idiomatic way there.

@bigbes
Copy link
Collaborator

bigbes commented Oct 21, 2025

Pls, add README.bench.md with:

  • instructions to run bench
  • your results of benches and you cpu/mem specs

@oleg-jukovec
Copy link
Collaborator

Pls, add README.bench.md with:

Probably we could just update the README.md instead of a separate document.

The README.md now is small and shouldn't become too big.

Closes TNTP-3733
@elhimov elhimov force-pushed the elhimov/tntp-3733-import-and-actualize-benchmarks branch from fdd30f4 to e6db0a0 Compare October 22, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants